Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fs: don't emit 'close' twice if emitClose enabled #31383

Closed
wants to merge 3 commits into from
Closed

fs: don't emit 'close' twice if emitClose enabled #31383

wants to merge 3 commits into from
+23 −1

Conversation

ronag
Copy link
Member

@ronag ronag commented Jan 16, 2020

fs streams have some backwards compat behavior that does not
behave well if emitClose: true is passed in options. This
fixes this edge case until the backwards compat is removed.

Fixes: #31366

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag
fs: don't emit 'close' twice if emitClose enabled
d388a35 
fs streams have some backwards compat behavior that does not
behave well if emitClose: true is passed in options. This
fixes this edge case until the backwards compat is removed.

Fixes: nodejs#31366
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jan 16, 2020
@ronag ronag requested a review from lundibundi January 16, 2020 17:53
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Trott
Trott approved these changes Jan 18, 2020
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I'd suggest a comment in the test to make it clear that it's testing that close is emitted once and not twice when emitClose is true. (But that's a nit. I'm fine with it as-is too.)

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28474/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28479/

Trott pushed a commit that referenced this pull request Jan 20, 2020
@ronag @Trott
fs: do not emit 'close' twice if emitClose enabled
7d5a86c 
fs streams have some backwards compat behavior that does not
behave well if emitClose: true is passed in options. This
fixes this edge case until the backwards compat is removed.

PR-URL: #31383
Fixes: #31366
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

Trott commented Jan 20, 2020

Landed in 7d5a86c

@Trott Trott closed this Jan 20, 2020
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@ronag @codebytere
fs: do not emit 'close' twice if emitClose enabled
2c2b3ba 
fs streams have some backwards compat behavior that does not
behave well if emitClose: true is passed in options. This
fixes this edge case until the backwards compat is removed.

PR-URL: #31383
Fixes: #31366
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 14, 2020
@ronag @codebytere
fs: do not emit 'close' twice if emitClose enabled
3f1212f 
fs streams have some backwards compat behavior that does not
behave well if emitClose: true is passed in options. This
fixes this edge case until the backwards compat is removed.

PR-URL: #31383
Fixes: #31366
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
@ronag @codebytere
fs: do not emit 'close' twice if emitClose enabled
fc9c6c3 
fs streams have some backwards compat behavior that does not
behave well if emitClose: true is passed in options. This
fixes this edge case until the backwards compat is removed.

PR-URL: #31383
Fixes: #31366
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@lpinca lpinca lpinca approved these changes

@lundibundi lundibundi Awaiting requested review from lundibundi

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Event 'close' emits twice (fs.createWriteStream)
4 participants
@ronag @nodejs-github-bot @Trott @lpinca